feat(ops): add B02 TSELS and TPRELU support - #2182
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR replaces ChangesSelection and PReLU operation updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant TileSelsWrapper
participant IR
participant PTOCodegen
Caller->>TileSelsWrapper: call sels(mask, src, tmp, scalar)
TileSelsWrapper->>IR: create tile.sels operation
IR->>PTOCodegen: emit four-operand tsels
PTOCodegen-->>Caller: generated operation
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4d7e26ec8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/pypto/ir/utils.py (1)
331-345: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSilent float→int truncation when
retype_constants=Trueon an integer operand.With retyping enabled, a float literal paired with an integer tile no longer takes the float fallback, so
_const_at_dtyperunsint(value):sels(mask, int32_tile, tmp, -1.5)silently becomes-1. Previously this combination surfaced as a dtype mismatch. Consider rejecting non-integral floats on integer targets instead of truncating.🛡️ Proposed guard
if target.is_float() or target.is_int(): + if target.is_int() and isinstance(value, float) and not float(value).is_integer(): + raise ValueError( + f"Scalar {value} cannot be represented exactly in {target}; " + "use an integer literal or cast the operand." + ) return _const_at_dtype(value, target, span)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/pypto/ir/utils.py` around lines 331 - 345, Update the constant-typing logic before _const_at_dtype so retype_constants=True does not convert a non-integral float to an integer target. Reject or preserve the existing dtype-mismatch behavior for float values with fractional parts when target.is_int(), while allowing integral floats and leaving other promotion paths unchanged.
🧹 Nitpick comments (1)
src/ir/op/tile_ops/elementwise.cpp (1)
918-932: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRows check uses physical extent while cols check uses valid extent.
required_rowsis compared againsttmp_type->shape_[0]butrequired_colsagainsttmp_valid_shape[1]. If that mixed contract is deliberate, a one-line comment would help; otherwise both should read from the same extent source. The literal8also duplicates the packed-predicate bits-per-byte constant used inMakePackedPredicateTileType; hoisting it to a file-scopeconstexprwould keep the two paths in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ir/op/tile_ops/elementwise.cpp` around lines 918 - 932, The UINT8 validation in the surrounding elementwise operator uses inconsistent physical versus valid extents and duplicates the packed-predicate bit-width literal. Align the row and column checks to the intended extent contract, document the choice if the difference is deliberate, and replace the literal 8 in required_cols and MakePackedPredicateTileType with one shared file-scope constexpr.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@python/pypto/ir/utils.py`:
- Around line 331-345: Update the constant-typing logic before _const_at_dtype
so retype_constants=True does not convert a non-integral float to an integer
target. Reject or preserve the existing dtype-mismatch behavior for float values
with fractional parts when target.is_int(), while allowing integral floats and
leaving other promotion paths unchanged.
---
Nitpick comments:
In `@src/ir/op/tile_ops/elementwise.cpp`:
- Around line 918-932: The UINT8 validation in the surrounding elementwise
operator uses inconsistent physical versus valid extents and duplicates the
packed-predicate bit-width literal. Align the row and column checks to the
intended extent contract, document the choice if the difference is deliberate,
and replace the literal 8 in required_cols and MakePackedPredicateTileType with
one shared file-scope constexpr.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 303255eb-bb59-47b7-830d-e72a1c4180c9
📒 Files selected for processing (19)
docs/en/dev/passes/31-memory_reuse.mddocs/en/dev/ptoas-op-status.mddocs/en/user/02-operation_reference.mddocs/zh-cn/dev/passes/31-memory_reuse.mddocs/zh-cn/dev/ptoas-op-status.mddocs/zh-cn/user/02-operation_reference.mdpython/pypto/debug/torch_codegen.pypython/pypto/ir/op/tile_ops.pypython/pypto/ir/utils.pypython/pypto/language/op/tile_ops.pysrc/backend/common/pto_ops_elementwise.cppsrc/ir/op/tile_ops/elementwise.cpptests/st/runtime/ops/test_activation_ops.pytests/st/runtime/ops/test_prelu.pytests/st/runtime/ops/test_sels.pytests/ut/codegen/test_pto_codegen_ops.pytests/ut/debug/test_torch_codegen.pytests/ut/ir/operators/test_tile_ops.pytests/ut/ir/transforms/test_memory_reuse.py
|
Review follow-up: 9dceb8d also rejects non-integral floating constants before integer retyping. The PReLU scratch check intentionally uses physical rows (the extra hardware scratch row) and valid columns (usable packed-predicate bytes); the shared 8-bit packing constant and an explanatory comment now make that distinction explicit. |
4489944 to
b21d263
Compare
|
Important 再次更新：最新评论 按 #2166 复核当前 head
请先修复这 11 个当前矩阵中的失败，并在仓库固定的 PTOAS v0.48 / PTO-ISA 上重跑真机矩阵；通过后再恢复 |
b4079f0 to
7956b51
Compare
|
已按该反馈复核并修复，当前 head 为
最新 CI 中 |
Little-oil
left a comment
Little-oil
left a comment
There was a problem hiding this comment.
结论：当前 head 7956b51b 暂不建议合入，下面 3 项需要先修复。
先校正旧结论：本次按仓库当前固定的 PTOAS v0.54（runtime/PTO-ISA 仍为 83d01313）复核。最新 A2/A3 真机任务 已全绿，B02 的 TPRELU 10 项、TSELS 27 项均通过。因此我之前基于旧 head b21d263d 提到的 11 项失败，以及旧 review 中“A2/A3 不支持整数 TSELS”的判断，都已过时；当前 INT16/UINT16/INT32/UINT32 路径有真机证据。
-
[blocker]
tile.sels没有验证 packed mask 是否足够覆盖src.valid_shape。DeduceTileSelsType目前只检查 mask 的 rank/dtype。例如mask=[1,32] ui8配src.valid_shape=[16,512]会通过 PyPTO/PTOAS verifier，但固定 PTO-ISA 会按 dst 的 16 个 valid rows 逐行读取 mask，而每行还需要覆盖 512 个 packed predicate bits；这会读出 mask 的实际存储范围，得到错误 predicate，甚至越界。请按 carrier 字节宽度验证 mask 的行覆盖和每行 packed-byte 容量（动态关系也应可证明），并补“过小 mask”负向 UT 与宽列边界 ST。 -
[blocker] TPRELU 的 alias/tmp 规则没有按 target 建模，当前同时有误拒和漏检。
args[0] != args[1]无条件拒绝相同 src/slope，后面的 8-bit tmp extent 检查及.not_inplace_safe()也全局套用。PTOAS v0.54 的 A5 实现不使用 tmp，并明确允许tmp == dst；A5 也没有 src/slope 不重叠限制，所以这些规则会拒绝合法 A5 形式。反过来，A3 要求 src/slope/tmp/dst 的存储范围两两不重叠，但同一 base 上两个重叠tile.slice是不同 Expr，会漏过当前判断。请在 target 和 MemRef 已知的阶段按 base+range 校验 A3，并补 A5 same-input/tmp-placeholder 正向用例、A3 overlapping-view 负向用例。 -
[G2] codegen UT 还不是 #2166 要求的 exact codegen 断言。
当前 TSELS/TPRELU 测试 只统计
ins数量并检查类型字符串；mask/src/tmp/scalar 换序或outs错误时仍可能通过。请断言完整的 SSA operand 顺序和outs，覆盖两个 target 分支。
另外请同步更新 PR 描述：其中仍写 PTOAS 0.48、A2/A3 仅 FP16/FP32、19 passed/22 deselected；当前事实是 PTOAS v0.54、A2/A3 还支持已真机通过的 16/32-bit integer TSELS，B02 当前为 37 项真机用例通过，A5 真机仍 pending。
13c1917 to
25daa7e
Compare
Little-oil
left a comment
Little-oil
left a comment
There was a problem hiding this comment.
结论：当前 head 25daa7e6 暂不建议合入。按 PTOAS v0.54（tag 195fdb8c）和 #2166 的 B02 / G0–G5 复核后，仍有 3 个 blocker。
先同步已完成项：我在上次 review 提到的 packed-mask 覆盖、TPRELU target-aware alias/tmp、exact SSA operand/outs 三项，在当前 head 都已实质修复；A2/A3 当前真机任务也全绿。下面只列当前仍存在的问题。
-
[blocker / G2] A5
TPRELU发出了 PTOAS v0.54 不存在的两输入形式。A5 codegen 只发
src, slope，layout 注册 也只处理两个输入。但 PTOAS v0.54 的 canonical ABI 在所有 target 上都固定为：pto.tprelu ins(%src0, %src1, %tmp : ...) outs(%dst : ...)
A5 “不使用
tmp”只表示实现不读写它，不表示可以从 IR 签名删除；tmp仍须是 Vec/row-major，但允许tmp == dst。我用仓库当前 pin 的ptoas 0.54实跑最小 IR：- 三输入且
tmp == dst：exit 0； - 删除第三个输入：exit 1，
error: expected ','。
当前 exact codegen UT 反而把错误的两输入形式写成了期望值。请让 A5 也发三个输入、给
tmp设置 row-major，并保留现有 A5tmp/result 可复用规则；中英文文档里的 “A5 omits tmp” 也需改成 “保留 operand，但实现不使用”。 - 三输入且
-
[blocker / G1] A2/A3
TSELS没有禁止tmp与mask/src的存储区间重叠，会静默算错。DeduceTileSelsType只检查tmp的 rank；.forbid_output_alias(0/2)只约束dst，不约束输入之间。因此下面两种调用都会通过并生成 PTO：pl.tile.sels(mask, src, src, scalar) # tmp == src pl.tile.sels(mask, src, mask, scalar) # tmp == mask
固定 A2/A3 PTO-ISA 会先执行
*scalarPtr = scalar，之后才读取mask/src做vsel：前者会污染src[0,0]，后者会污染 mask 开头。请在 A2/A3 backend、MemRef 已知后，按 base+range 禁止tmp与mask/srcoverlap，并补同一变量及 overlapping slice 的负向 UT。A5 不读取tmp，应继续允许这些 alias。 -
[blocker / G1] A2/A3
TPRELU把动态 scratch 容量关系的kUnknown当成合法。rows/cols 两项检查 都使用
proof != kFalse，所以“无法证明安全”的动态关系会被放行。例如：src/slope shape=[16,256]，共同 runtimevalid_shape=[M,N]；tmp shape=[2,32]，valid_shape=[1,1]。
编译时
M + 1 <= 2、ceil(N / 8) <= 1都是kUnknown，当前代码仍会 emit；运行M=16, N=256时则同时违反两项 scratch 契约。固定 ISA 会按dst.validRow在 tmp 上创建 额外 scratch row 的TSUBVIEW，此时可能越界或产生错误 predicate。请要求关系可证明安全，或使用 physical 上界/runtime guard，并补动态负向 UT；不能只排除kFalse。 -
[should-fix / G1–G2] TSELS 的 packed-capacity UT 把 PTOAS 无法组装的物理行宽当成正例。
当前 UT 接受
i16[2,17]（每行 34B）和i32[2,9]（每行 36B）。我用 v0.54 实跑对应pto.alloc_tile，会直接报：expects result row-major none_box tile row byte size ... to be 32-byte aligned这项宜在实际 layout/MemRef 已知后按 physical stride 校验，避免简单按逻辑 cols 检查而误拒有对齐 backing stride 的 subview。至少不要把 34B/36B 形式作为“可执行正例”；可改为对齐 physical shape + 较小 valid shape，并增加 pinned PTOAS assembly 断言。
-
[G3 / G5] 请校正 A5 覆盖声明和当前计数。
system-tests-a5sim没有运行test_prelu.py或test_sels.py，这也是上述 A5 ABI 错误在 required CI 全绿时未被发现的原因。因此 PR 描述中的 “A5 simulation: covered by CI” 目前不成立：请把两份 ST 真正接入 A5sim，或明确标为 pending。另一个小的状态更新是，当前 A2/A3 direct job 实际为TPRELU 10 + TSELS 29 = 39项通过，不是正文中的 37 项。
修复后建议至少补三条验收证据：A5 三输入 pto.tprelu 通过 v0.54 组装；A2/A3 TSELS overlap 被 PyPTO 提前拒绝；动态 TPRELU 小 scratch 不再以 kUnknown 放行。
b8cf966 to
78c36af
Compare
Little-oil
left a comment
Little-oil
left a comment
There was a problem hiding this comment.
结论：当前 head 78c36afd 暂不建议合入。按 PTOAS v0.55 和 #2166 的 B02 / G0–G5 重新复核后，上次 review 的 3 个 blocker（A5 TPRELU 三输入 ABI、A2/A3 TSELS 的 tmp-mask/tmp-src overlap、动态 TPRELU scratch 的 kUnknown）以及未对齐 packed-capacity 正例都已经修复。当前还剩下面 3 项。
-
[blocker / G1] A2/A3
TSELS误拒了合法的tmp == dst。当前 backend 检查 把
dst也放进tmp的禁止重叠集合，memory reuse 也禁止 output 复用 arg 2。但 PTOAS v0.55 官方 TSELS sample 明确调用TSelSOp(tb0, tb1, tb2, c64, tb2)，即tmp与dst是同一 tile；我用ptoas 0.55对该形式在a2、a3都实跑组装成功。这也符合固定 PTO-ISA 的执行顺序：先把 scalar 写进 tmp、
set_cmpmask并做 barrier，之后才循环写 dst。因此应继续禁止tmp与mask/srcoverlap，但允许tmp == dst；同时删除 memory-reuse 的 arg 2 禁止项，并更新文档/UT，补一个官方同形的 A2/A3 正向 assembly case。 -
[G3 / G5] A5sim job 实际执行了 0 个 TPRELU/TSELS case，PR 正文的覆盖声明不成立。
Workflow 的确把两个文件写进了命令，但测试全部参数化
ONBOARD_PLATFORMS（只有a2a3/a5），而 job 传入--platform=a5sim。当前 green job 因此是217 collected / 188 deselected / 29 selected，执行列表里没有任何test_prelu.py::或test_sels.py::。请二选一：把相应参数扩展到
a5sim并确认日志出现实际 B02 node id；或者把 A5 simulation 明确标为 pending，不再把这个 green job 当作 B02 的 A5 证据。 -
[G2 / test] 删除
[1,1] UINT8这个 A5 正向测试输入。test_tprelu_undersized_tmp_is_a3_only_validation在 A5 分支把[1,1] UINT8当作可生成正例，但 PTOAS v0.55 会在pto.alloc_tile阶段直接拒绝：row byte size ... must be 32-byte aligned, but got 1 bytes，尚未进入 TPRELU verifier。按当前工具链约束，先不要把这种形态作为测试用例；请删除它，或改成行宽对齐的占位 tile（例如[1,32] UINT8）并让生成结果实际通过 v0.55 组装。
版本说明：PR/required CI 目前仍固定 PTOAS v0.54，正文还引用 final rebase 前的 v0.53 真机结果；这些都不能作为当前 v0.55 基线的证据。我在 v0.55 下确认 canonical 三输入 TPRELU、对齐的 A5 [1,32] UINT8 placeholder，以及 A2/A3 TSELS tmp == dst 都可组装。修复后请用 v0.55 对最终 head 刷新版本和验证记录。
78c36af to
3d7f0fa
Compare
Little-oil
left a comment
Little-oil
left a comment
There was a problem hiding this comment.
Important
更正：撤回我在上一条 review中的第 1 项 blocker 和 v0.55 强制刷新要求。 请不要继续把这两项当作 #2166 的合入门槛。
-
A2/A3
TSELS tmp == dst不应被我定为 G1 blocker。我只依据 PTOAS sample、assembler 接受和固定 ISA 的指令顺序，就断言 PyPTO “必须”允许该 alias；这个推论越过了证据边界。sample 使用同一个 tile SSA 且只做构造/verify，assembler 成功也只说明 verifier 没拦。PTOAS v0.54/v0.55 的
TSelSOp::getEffects在 A2/A3 上把tmp与dst都建模为 write，memory planner 还会为 scratch/output 建立 forbid-alias conflict；#2166 B02 也没有要求 TSELS 接受这个自动复用。因此，旧的保守 no-alias 保护是可接受的，不是 correctness blocker。当前 head 已按我的错误反馈放开自动复用；最小纠正是恢复旧保护。如果选择保留这项优化，则需要补一条明确迫使
tmp/result同地址的 A2/A3 真机数值 ST，并说明它与 PTOAS planner 保守规则的关系；但这项优化不是完成 B02 的前置要求。 -
A5sim 的事实成立，但严重性应降级。
当前 A5sim job 确实执行了 0 个 TPRELU/TSELS case，PR 正文 “A5 simulation: covered by CI” 应修正；但 #2166 G3 只要求至少一个支持平台完成真机 ST，已有 A2/A3 真机证据可以满足，A5 保持 pending 即可。这不是否定 A2/A3
✅的 blocker。 -
[1,1] UINT8测试问题仍然成立，而且应统一清理。PTOAS v0.54/v0.55 都会在
pto.alloc_tile阶段因 1-byte row 不满足 32-byte alignment 而拒绝。当前有三处这种测试输入：TSELScodegen、TPRELUA5 正向 codegen 和TPRELUIR。请删除不需要的 case，或统一换成当前 A5 EmitC 路径可组装、且仍能表达“小 placeholder”的[1,32] UINT8。其中 A5 正向 codegen case 是直接的 G2 must-fix。 -
仓库 pin v0.54 才是本 PR 的验收版本。
PR/base 当前仍 pin PTOAS v0.54；#2166 的 G0/G2 要求按 pinned 版本验收，并要求工具链升级单独提交。v0.55 只应作为额外的前向兼容检查，不能要求 #2182 顺带刷新 pin/CI。
修正后的结论： 原 review 中唯一直接成立的代码/测试 blocker 是无效的 [1,1] UINT8 A5 正向用例；A5sim 覆盖声明和旧版本验证文字属于状态/文案修正。TSELS tmp/dst 自动复用不应由我的错误 review 强制加入。
3d7f0fa to
d216b04
Compare
d216b04 to
d366752
Compare
3 participants
Summary
tile.selspath with the canonicalmask, src, tmp, scalarTSELS contracttile.prelu(src, slope, tmp)inference, backend emission, alias rules, debug codegen, and bilingual documentationValidation
78c36afd0b9260b86313f4ba514462985c140964cmake --build build --parallel 1passedTPRELU10,TSELS29) with PTOAS v0.53Platform status